-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[hack pipes] Inline topic token when possible #14278
[hack pipes] Inline topic token when possible #14278
Conversation
|
||
return _ref4 = n, (_ref3 = Math.abs(_ref4), (_ref2 = Promise.resolve(_ref3), (_ref = await _ref2, triple(_ref)))); | ||
return _ref = Math.abs(n), triple(await Promise.resolve(_ref)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't fully inlined because Promise.resolve
might be an impure getter. Since Babel assumes unmodified builtins, in Babel 8 we could expand this "implicit assumptions" to also mark this as pure and transform it to
return triple(await Promise.resolve(Math.abs(n)));
@@ -1,7 +1,7 @@ | |||
let i = 0; | |||
let sum = 0; | |||
|
|||
while (i |> (i = ^ + 1) |> ^ <= 10) | |||
while (i |> (i = 2 * ^ - ^ + 1) |> ^ <= 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to keep the "spirit" of the test, otherwise it was completely inlined.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51308/ |
!state.sideEffectsBeforeFirstTopicReference && | ||
!path.isPure() | ||
) { | ||
state.sideEffectsBeforeFirstTopicReference = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sideEffectsBeforeFirstTopicReference invalidated only when side effects occur before the first topic reference in an RHS? For example, will it be set to true for x |> (f(x), g(x))
x |> (f(#), g(#))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean x |> (f(#), g(#))
and that f
/g
are defined and not reassigned. It will not be set to true because there are no side effects before the first #
. However, since there are multiple #
s we won't inline anything.
I only tracked the side effects before the first one, so that x |> (#, SIDE_EFFECT)
doesn't prevent inlining.
} | ||
} | ||
}, | ||
"ClassBody|Function"(_, state) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class body does not encapsulate all expressions under class, for example: class decorators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly why I used ClassBody: class decorators evaluation is not deferred!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so then we have to exclude computed class element keys, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that exactly determining what runs "now" is hard. The list includes (but is not limited to):
- class keys
- static fields
- iifes
To avoid making the transform too complex, I'd prefer to leave it as-is and deoptimize when they happen.
I discovered that "removing unnecessary tmp vars" is quite satisfying 😁
When a topic token is only used once and either:
|>
is pure|>
before evaluating the topic token are purethe pipeline operator can be compiled without temporary variables, by directly inlining the lhs in the rhs.
cc @js-choi if you have time to review, since you initially implemented this plugin!